feat(desktop): modifier-keyed v2 terminal file links + folder sidebar reveal#3512
Merged
saddlepaddle merged 12 commits intomainfrom Apr 18, 2026
Merged
feat(desktop): modifier-keyed v2 terminal file links + folder sidebar reveal#3512saddlepaddle merged 12 commits intomainfrom
saddlepaddle merged 12 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors the terminal pane link-handling mechanism by extracting file-opening logic from the component into externally-provided callbacks. The parent workspace component now manages file open, reveal path, and external editor actions, while the terminal pane delegates to these callbacks rather than making direct tRPC mutations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR wires the existing Terminal file links setting ( The change is contained to a single file. Key design decisions are sound: using the imperative Notable observations:
Confidence Score: 4/5Safe to merge — the core logic is correct and the setting is respected; two non-blocking issues remain. The behavior switch is logically correct (equivalent condition to v1), event.preventDefault() is called before the await, the fallback default matches the app constant, and v1 is untouched. Two P1/P2 items prevent a 5: the link.row/link.col discrepancy is a real capability regression vs. v1's stated matching behavior, and the interface type is stale. Only TerminalPane.tsx changed; terminal-link-manager.ts should also be updated to reflect the async handler signature.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | Adds terminal link behavior setting support to v2 TerminalPane. Logic is correct; onFileLinkClick is now async but the shared interface still declares it as void, and link.row/link.col are not forwarded to FilePane. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Line: 148
Comment:
**`onFileLinkClick` interface type not updated for async**
`TerminalLinkHandlers.onFileLinkClick` is declared as `(event: MouseEvent, link: DetectedLink) => void` in `terminal-link-manager.ts`, but the handler here now returns `Promise<void>`. TypeScript accepts the assignment because `Promise<void>` is structurally compatible with `void`, and xterm's `activate` callback discards the return value — so this works at runtime. But the interface no longer accurately documents the contract, and any future consumer that inspects the return type could be surprised.
Consider updating the interface to be explicit:
```ts
// terminal-link-manager.ts
onFileLinkClick?: (event: MouseEvent, link: DetectedLink) => void | Promise<void>;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Line: 156-166
Comment:
**`link.row` / `link.col` discarded — no line navigation in FilePane**
`DetectedLink` carries `row` and `col` parsed from link suffixes like `src/app.ts:42:5`. The v1 implementation explicitly forwards these for cursor positioning:
```ts
// useFileLinkClick.ts (v1)
addFileViewerPane(workspaceId, { filePath: resolvedPath, line, column });
```
The v2 implementation silently discards them:
```ts
data: {
filePath: link.resolvedPath,
mode: "editor",
hasChanges: false,
} satisfies FilePaneData,
```
`FilePaneData` doesn't currently have `line`/`column` fields, so the fix requires a schema change upstream. As-is, clicking `error.ts:42` in a v2 terminal opens the file but drops the user at the top rather than line 42. The PR description states this "matches v1", but v1 does navigate to the line. This is worth tracking as a follow-up even if it's out of scope for this PR.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(desktop): v2 terminal honors termin..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:154">
P3: Avoid silently swallowing `getTerminalLinkBehavior` query failures; log the error before defaulting so behavior mismatches are diagnosable.
(Based on your team's feedback about handling async failures explicitly instead of silently swallowing them.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
saddlepaddle
force-pushed
the
prefer-open-superset-file-editor
branch
from
21d8250 to
35457b1
Compare
saddlepaddle
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 162-177: The onFileLinkClick handler currently ignores the
terminal link setting and uses a shift-key heuristic; update onFileLinkClick to
asynchronously call electronTrpcClient.settings.getTerminalLinkBehavior.query()
on each click (with fallback "file-viewer" on error) and then route: if behavior
=== "external-editor" or (event.shiftKey && behavior !== "file-viewer") call
onOpenExternalRef.current(link.resolvedPath, {line: link.row, column:
link.col}); if behavior === "file-viewer" open in-app via
onOpenFileRef.current(link.resolvedPath); keep the existing directory handling
with onRevealPathRef.current(link.resolvedPath); ensure errors default to
"file-viewer" and preserve event.preventDefault() semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27a8bc55-80d5-4481-99c3-d317a575cb0c
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
Comment on lines
+162
to
177
| onFileLinkClick: (event, link) => { | ||
| if (!event.metaKey && !event.ctrlKey) return; | ||
| event.preventDefault(); | ||
| if (event.shiftKey) { | ||
| onOpenExternalRef.current(link.resolvedPath, { | ||
| line: link.row, | ||
| column: link.col, | ||
| }) | ||
| .catch((error) => { | ||
| console.error("[v2 Terminal] Failed to open file:", error); | ||
| toast.error("Failed to open file in editor"); | ||
| }); | ||
| return; | ||
| } | ||
| if (link.isDirectory) { | ||
| onRevealPathRef.current(link.resolvedPath); | ||
| } else { | ||
| onOpenFileRef.current(link.resolvedPath); | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that terminalLinkBehavior is not referenced anywhere in the v2 terminal link path
rg -nP --type=ts --type=tsx -C2 'terminalLinkBehavior|getTerminalLinkBehavior'
# And confirm the settings router exposes it (sanity check)
rg -nP --type=ts -C2 'getTerminalLinkBehavior' -g '**/trpc/**'Repository: superset-sh/superset
Length of output: 553
🏁 Script executed:
# First, find the exact file path and read the TerminalPane.tsx to see the full context
find . -name "TerminalPane.tsx" -type f | head -20Repository: superset-sh/superset
Length of output: 213
🏁 Script executed:
# Read the TerminalPane.tsx file to understand the full implementation
# Specifically looking for getTerminalLinkBehavior calls and the onFileLinkClick handler
fd -e tsx "TerminalPane.tsx" --exec cat {} \;Repository: superset-sh/superset
Length of output: 7397
🏁 Script executed:
# Also search for any references to getTerminalLinkBehavior or terminalLinkBehavior in the v2 workspace context
rg -t ts 'terminalLinkBehavior|getTerminalLinkBehavior' apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/ -C3Repository: superset-sh/superset
Length of output: 46
Setting terminalLinkBehavior is never consulted — feature appears unimplemented.
The PR objective states that v2 terminal link clicks should honor the Settings → Terminal → Terminal file links preference by reading it via electronTrpcClient.settings.getTerminalLinkBehavior.query() on each click, with "file-viewer" as the default/error fallback. The onFileLinkClick handler (lines 162–177) never reads this setting: it uses a shift-modifier heuristic instead (shift ⇒ external, else ⇒ in-app). As a result, a user who has set Terminal file links to "external-editor" will still see files open in the FilePane on a plain Cmd/Ctrl-click — the opposite of what the setting requests.
Please clarify whether the setting-lookup approach was intentionally replaced with a shift modifier, or if the getTerminalLinkBehavior query is missing. If the latter, it should be called inside onFileLinkClick (async context), routing to onOpenFile vs onOpenExternal based on the resolved setting value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
around lines 162 - 177, The onFileLinkClick handler currently ignores the
terminal link setting and uses a shift-key heuristic; update onFileLinkClick to
asynchronously call electronTrpcClient.settings.getTerminalLinkBehavior.query()
on each click (with fallback "file-viewer" on error) and then route: if behavior
=== "external-editor" or (event.shiftKey && behavior !== "file-viewer") call
onOpenExternalRef.current(link.resolvedPath, {line: link.row, column:
link.col}); if behavior === "file-viewer" open in-app via
onOpenFileRef.current(link.resolvedPath); keep the existing directory handling
with onRevealPathRef.current(link.resolvedPath); ensure errors default to
"file-viewer" and preserve event.preventDefault() semantics.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (2)
158-164: Shift+Cmd on a directory passesline/columnthat are meaningless.When
link.isDirectoryis true, the shift-branch still forwards{ line: link.row, column: link.col }toonOpenExternal. External editors will typically ignore these for a directory open, but it's cleaner to omit them (and avoids surprising editor behavior that could, e.g., try to open the dir as a file at that position).Proposed tweak
- onFileLinkClick: (event, link) => { - if (!event.metaKey && !event.ctrlKey) return; - event.preventDefault(); - if (event.shiftKey) { - onOpenExternal(link.resolvedPath, { - line: link.row, - column: link.col, - }); - return; - } - if (link.isDirectory) { - onRevealPath(link.resolvedPath); - } else { - onOpenFile(link.resolvedPath); - } - }, + onFileLinkClick: (event, link) => { + if (!event.metaKey && !event.ctrlKey) return; + event.preventDefault(); + if (event.shiftKey) { + onOpenExternal( + link.resolvedPath, + link.isDirectory + ? undefined + : { line: link.row, column: link.col }, + ); + return; + } + if (link.isDirectory) { + onRevealPath(link.resolvedPath); + } else { + onOpenFile(link.resolvedPath); + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx around lines 158 - 164, The shift+click branch forwards line/column even when link.isDirectory is true; update the shift branch in TerminalPane (where event.shiftKey is checked and onOpenExternal is called) to omit the {line: link.row, column: link.col} payload when link.isDirectory is true—call onOpenExternal(link.resolvedPath) for directories and keep the existing onOpenExternal(link.resolvedPath, {line: link.row, column: link.col}) for non-directories so editors don't receive meaningless file-position args for directories.
31-36:openInNewTabparameter is declared but never used at the call site.
onOpenFileis typed as(path: string, openInNewTab?: boolean) => void, but line 168 only passesresolvedPath. If there's no terminal-driven use case for new-tab behavior here, consider narrowing the prop type to(path: string) => voidto avoid a misleading API surface; otherwise, decide the modifier-key mapping and wire it through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx around lines 31 - 36, The prop type for onOpenFile currently allows a second optional openInNewTab boolean but the call site in TerminalPane only passes resolvedPath (so the parameter is unused/misleading); either narrow the prop signature to onOpenFile: (path: string) => void or propagate a true/false value based on a modifier-key mapping (e.g., map Alt/Cmd key state in the TerminalPane event handler) and pass that as the second argument when calling onOpenFile(resolvedPath, openInNewTab). Update the onOpenFile type and the TerminalPane call site consistently (or wire modifier-key detection into the TerminalPane handler) so the API and usage match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 158-164: The shift+click branch forwards line/column even when
link.isDirectory is true; update the shift branch in TerminalPane (where
event.shiftKey is checked and onOpenExternal is called) to omit the {line:
link.row, column: link.col} payload when link.isDirectory is true—call
onOpenExternal(link.resolvedPath) for directories and keep the existing
onOpenExternal(link.resolvedPath, {line: link.row, column: link.col}) for
non-directories so editors don't receive meaningless file-position args for
directories.
- Around line 31-36: The prop type for onOpenFile currently allows a second
optional openInNewTab boolean but the call site in TerminalPane only passes
resolvedPath (so the parameter is unused/misleading); either narrow the prop
signature to onOpenFile: (path: string) => void or propagate a true/false value
based on a modifier-key mapping (e.g., map Alt/Cmd key state in the TerminalPane
event handler) and pass that as the second argument when calling
onOpenFile(resolvedPath, openInNewTab). Update the onOpenFile type and the
TerminalPane call site consistently (or wire modifier-key detection into the
TerminalPane handler) so the API and usage match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1344f7c-95da-4b26-a0c8-e1a79497a04b
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx (2)
229-234: Nit: log prefix misattributes origin.
openExternalis now a workspace-scoped action exposed viaPaneActionsProviderto any pane, but the error prefix still reads"[v2 Terminal]". Consider"[v2 Workspace]"(or similar) so logs stay accurate if non-terminal consumers are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx around lines 229 - 234, The console error prefix incorrectly attributes the failure to the terminal; in the catch handler for openExternal (the promise catcher where console.error and toast.error are called) update the log prefix from "[v2 Terminal]" to a workspace-scoped label like "[v2 Workspace]" (or similar) so the log reflects that openExternal is a PaneActionsProvider/workspace-scoped action; ensure the message passed to console.error remains descriptive and include the caught error variable.
389-395: Memoize thePaneActionsProvidervalue to avoid re-rendering every consumer each render.
value={{ onOpenFile, onRevealPath, onOpenExternal }}is a fresh object on everyWorkspaceContentrender, so allusePaneActions()consumers (currently everyTerminalPane) re-render on every parent render even though the three callbacks themselves are stable viauseCallback.♻️ Proposed fix
+ const paneActions = useMemo( + () => ({ + onOpenFile: openFilePane, + onRevealPath: revealPath, + onOpenExternal: openExternal, + }), + [openFilePane, revealPath, openExternal], + ); + return ( <FileDocumentStoreProvider workspaceId={workspaceId}> - <PaneActionsProvider - value={{ - onOpenFile: openFilePane, - onRevealPath: revealPath, - onOpenExternal: openExternal, - }} - > + <PaneActionsProvider value={paneActions}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx around lines 389 - 395, The PaneActionsProvider is being passed a new object each render which forces downstream consumers to re-render; in WorkspaceContent memoize the provider value by creating a stable object (e.g., useMemo(() => ({ onOpenFile: openFilePane, onRevealPath: revealPath, onOpenExternal: openExternal }), [openFilePane, revealPath, openExternal])) so the object identity stays stable while still referencing the existing useCallback handlers (openFilePane, revealPath, openExternal) before passing it to <PaneActionsProvider value={...}>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 229-234: The console error prefix incorrectly attributes the
failure to the terminal; in the catch handler for openExternal (the promise
catcher where console.error and toast.error are called) update the log prefix
from "[v2 Terminal]" to a workspace-scoped label like "[v2 Workspace]" (or
similar) so the log reflects that openExternal is a
PaneActionsProvider/workspace-scoped action; ensure the message passed to
console.error remains descriptive and include the caught error variable.
- Around line 389-395: The PaneActionsProvider is being passed a new object each
render which forces downstream consumers to re-render; in WorkspaceContent
memoize the provider value by creating a stable object (e.g., useMemo(() => ({
onOpenFile: openFilePane, onRevealPath: revealPath, onOpenExternal: openExternal
}), [openFilePane, revealPath, openExternal])) so the object identity stays
stable while still referencing the existing useCallback handlers (openFilePane,
revealPath, openExternal) before passing it to <PaneActionsProvider
value={...}>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd951fb7-ad37-4cf9-acdc-6a9f3525ab31
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/providers/PaneActionsProvider/PaneActionsProvider.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/providers/PaneActionsProvider/index.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/providers/PaneActionsProvider/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 220-237: The guard in openExternal currently allows the mutation
when workspaceHost is undefined or machineId is null during initial load; change
the conditional in the openExternal callback (the function named openExternal)
to treat missing data as "not local" by short-circuiting when workspaceHost is
falsy or machineId is falsy or when workspaceHost.hostMachineId !== machineId
(e.g., if (!workspaceHost || !machineId || workspaceHost.hostMachineId !==
machineId) { toast.error(...); return; }), so external.openFileInEditor.mutate
is only called when both host and local machineId are present and match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7217e80d-994c-4630-a765-3ee151be108a
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts:34">
P1: The remote-workspace check is bypassed while host data is still loading, so Cmd/Ctrl+Shift-click can open externally before locality is confirmed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsx:51">
P2: Don't silently swallow `getDefaultEditor` query failures; log a warning so settings RPC failures remain observable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
When the user's "Terminal file links" setting is "file-viewer", Cmd/Ctrl-clicking a file path in a v2 workspace terminal now opens the file in an in-app FilePane instead of the external editor — matching the v1 behavior the setting already controls. Directories and the "external-editor" setting continue to fall through to the existing openFileInEditor path.
… reveal Replaces the settings-based branch with a modifier-key pattern: - Cmd/Ctrl-click a file path → opens in an in-app FilePane. - Cmd/Ctrl+Shift-click a file or directory → opens in the external editor (with an upfront toast for remote workspaces, same pattern as FilesTab's Open in editor guard). - Cmd/Ctrl-click a directory path → force-opens the sidebar, reveals the folder in the file tree (ancestors expand, row scrolls into view and highlights). Implementation reuses the existing selectedFilePath → fileTree.reveal machinery in FilesTab by promoting selectedFilePath from a pane-store derivation to a useState, synced from the active file pane via useEffect. Folder focus is just a direct setSelectedFilePath — the existing sidebar code path handles reveal + scroll + highlight without changes. Folder paths now also flow through getParentForCreation, so the "New File" toolbar button creates inside the focused folder. Three callbacks (onOpenFile / onRevealPath / onOpenExternal) are plumbed from page.tsx through usePaneRegistry to TerminalPane. The shift-modifier path goes through openExternal, which checks workspaceHost.hostMachineId !== machineId and toasts for remote workspaces instead of firing a mutation the remote won't satisfy. v1 code untouched; DB schema untouched; v2 settings UI untouched (terminalLinkBehavior still honored by v1).
…ntext Removes the terminal-specific callback plumbing from usePaneRegistry (which should only care about how to render each pane kind) and moves onOpenFile / onRevealPath / onOpenExternal into a React context scoped to the workspace page. TerminalPane consumes via usePaneActions() instead of taking them as props.
…PaneRegistry The context indirection wasn't worth it for a single consumer (TerminalPane). Passing the three callbacks through usePaneRegistry options is simpler and has no actual downside since usePaneRegistry is only called from one place anyway.
Previously revealing a directory only toggled rightSidebarOpen + setSelectedFilePath, but the sidebar would stay on whichever tab the user had last (e.g., Changes/Review), leaving FilesTab unmounted so the reveal effect never fired. Update the same v2WorkspaceLocalState transaction to also force sidebarState.activeTab back to "files".
…nalEditor - useFileTree.reveal now also expands the target itself when it's a directory, using stateRef so there's no staleness concern. All reveal call sites benefit. - Extract useOpenInExternalEditor hook (remote check + toast + mutate) so TerminalPane can consume it directly instead of through a callback. Drops one prop from usePaneRegistry and removes the local workspaceHost liveQuery from page.tsx. FilesTab's handleOpenInEditor could migrate to the same hook in a follow-up to dedupe the pattern.
Cmd/Ctrl-click a URL in a v2 terminal now opens a BrowserPane in the current tab. Cmd/Ctrl+Shift-click still opens in the external browser. Widens TerminalLinkHandlers.onUrlClick to receive the MouseEvent (v1's helper just threads it through unused — behavior unchanged).
…ction Adds onLinkHover/onLinkLeave callbacks to TerminalLinkHandlers, wired through LinkDetectorAdapter, UrlLinkProvider, and WordLinkDetector so every detected link participates. In v2 TerminalPane, a new LinkHoverTooltip component tracks hover + live modifier state (global keydown/keyup listeners scoped to hover duration) and portals a positioned tooltip to document.body when meta/ctrl is held. Content flips on shift: - File: Open in editor | shift: Open externally - Folder: Reveal in sidebar | shift: Open externally - URL: Open in browser | shift: Open in external browser v1's helpers.ts doesn't opt into the new callbacks, so v1 hover behavior is unchanged.
- Tooltip now uses the same bg-foreground/text-background/rounded-md/px-3/py-1.5/text-xs tokens as the project's TooltipContent, so it visually matches the rest of the app (was a custom border/popover style before). - Shift-modifier tooltip text now says "Open in Cursor" / "Open in VS Code" / etc. based on the user's configured defaultEditor setting, resolved via electronTrpcClient.settings.getDefaultEditor + getAppOption display label. Falls back to "Open externally" if no editor is configured.
…ifier listener - Move useLinkHoverState into its own hooks/ folder (was exported alongside the component in violation of the one-component-per-file rule). - Effect now re-subscribes on hover start/end only (deps: hovering boolean), not on every modifier change. - Filter to Meta/Control/Shift/Alt key events so typing a letter while hovering doesn't churn state. - Skip setState when modifier/shift values didn't actually change, avoiding identity-change re-renders on repeat keydowns. - Extract tooltip offset constant.
saddlepaddle
force-pushed
the
prefer-open-superset-file-editor
branch
from
6220e77 to
51f1787
Compare
…or-query failures - useOpenInExternalEditor now treats an unloaded workspaceHost as non-local (workspaceHost?.hostMachineId !== machineId) so Cmd+Shift-click doesn't fire the mutation against a potentially remote workspace before locality is confirmed. - LinkHoverTooltip's getDefaultEditor catch now console.warn's the error before falling back to null, so settings RPC failures stay observable.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 18, 2026
… reveal (superset-sh#3512) * feat(desktop): v2 terminal honors terminalLinkBehavior setting When the user's "Terminal file links" setting is "file-viewer", Cmd/Ctrl-clicking a file path in a v2 workspace terminal now opens the file in an in-app FilePane instead of the external editor — matching the v1 behavior the setting already controls. Directories and the "external-editor" setting continue to fall through to the existing openFileInEditor path. * feat(desktop): modifier-keyed v2 terminal file links + folder sidebar reveal Replaces the settings-based branch with a modifier-key pattern: - Cmd/Ctrl-click a file path → opens in an in-app FilePane. - Cmd/Ctrl+Shift-click a file or directory → opens in the external editor (with an upfront toast for remote workspaces, same pattern as FilesTab's Open in editor guard). - Cmd/Ctrl-click a directory path → force-opens the sidebar, reveals the folder in the file tree (ancestors expand, row scrolls into view and highlights). Implementation reuses the existing selectedFilePath → fileTree.reveal machinery in FilesTab by promoting selectedFilePath from a pane-store derivation to a useState, synced from the active file pane via useEffect. Folder focus is just a direct setSelectedFilePath — the existing sidebar code path handles reveal + scroll + highlight without changes. Folder paths now also flow through getParentForCreation, so the "New File" toolbar button creates inside the focused folder. Three callbacks (onOpenFile / onRevealPath / onOpenExternal) are plumbed from page.tsx through usePaneRegistry to TerminalPane. The shift-modifier path goes through openExternal, which checks workspaceHost.hostMachineId !== machineId and toasts for remote workspaces instead of firing a mutation the remote won't satisfy. v1 code untouched; DB schema untouched; v2 settings UI untouched (terminalLinkBehavior still honored by v1). * refactor(desktop): drop callback refs in v2 TerminalPane, use effect deps directly * refactor(desktop): move v2 pane actions into a PaneActionsProvider context Removes the terminal-specific callback plumbing from usePaneRegistry (which should only care about how to render each pane kind) and moves onOpenFile / onRevealPath / onOpenExternal into a React context scoped to the workspace page. TerminalPane consumes via usePaneActions() instead of taking them as props. * refactor(desktop): drop PaneActionsProvider, pass actions through usePaneRegistry The context indirection wasn't worth it for a single consumer (TerminalPane). Passing the three callbacks through usePaneRegistry options is simpler and has no actual downside since usePaneRegistry is only called from one place anyway. * fix(desktop): v2 terminal folder reveal switches sidebar to Files tab Previously revealing a directory only toggled rightSidebarOpen + setSelectedFilePath, but the sidebar would stay on whichever tab the user had last (e.g., Changes/Review), leaving FilesTab unmounted so the reveal effect never fired. Update the same v2WorkspaceLocalState transaction to also force sidebarState.activeTab back to "files". * fix(desktop): auto-expand revealed directory + extract useOpenInExternalEditor - useFileTree.reveal now also expands the target itself when it's a directory, using stateRef so there's no staleness concern. All reveal call sites benefit. - Extract useOpenInExternalEditor hook (remote check + toast + mutate) so TerminalPane can consume it directly instead of through a callback. Drops one prop from usePaneRegistry and removes the local workspaceHost liveQuery from page.tsx. FilesTab's handleOpenInEditor could migrate to the same hook in a follow-up to dedupe the pattern. * feat(desktop): v2 terminal URL links open in internal browser by default Cmd/Ctrl-click a URL in a v2 terminal now opens a BrowserPane in the current tab. Cmd/Ctrl+Shift-click still opens in the external browser. Widens TerminalLinkHandlers.onUrlClick to receive the MouseEvent (v1's helper just threads it through unused — behavior unchanged). * feat(desktop): v2 terminal shows hover tooltip describing cmd-click action Adds onLinkHover/onLinkLeave callbacks to TerminalLinkHandlers, wired through LinkDetectorAdapter, UrlLinkProvider, and WordLinkDetector so every detected link participates. In v2 TerminalPane, a new LinkHoverTooltip component tracks hover + live modifier state (global keydown/keyup listeners scoped to hover duration) and portals a positioned tooltip to document.body when meta/ctrl is held. Content flips on shift: - File: Open in editor | shift: Open externally - Folder: Reveal in sidebar | shift: Open externally - URL: Open in browser | shift: Open in external browser v1's helpers.ts doesn't opt into the new callbacks, so v1 hover behavior is unchanged. * refactor(desktop): match tooltip styling, surface configured editor name - Tooltip now uses the same bg-foreground/text-background/rounded-md/px-3/py-1.5/text-xs tokens as the project's TooltipContent, so it visually matches the rest of the app (was a custom border/popover style before). - Shift-modifier tooltip text now says "Open in Cursor" / "Open in VS Code" / etc. based on the user's configured defaultEditor setting, resolved via electronTrpcClient.settings.getDefaultEditor + getAppOption display label. Falls back to "Open externally" if no editor is configured. * refactor(desktop): split LinkHoverTooltip hook/component, tighten modifier listener - Move useLinkHoverState into its own hooks/ folder (was exported alongside the component in violation of the one-component-per-file rule). - Effect now re-subscribes on hover start/end only (deps: hovering boolean), not on every modifier change. - Filter to Meta/Control/Shift/Alt key events so typing a letter while hovering doesn't churn state. - Skip setState when modifier/shift values didn't actually change, avoiding identity-change re-renders on repeat keydowns. - Extract tooltip offset constant. * fix(desktop): block external open while host data loads, surface editor-query failures - useOpenInExternalEditor now treats an unloaded workspaceHost as non-local (workspaceHost?.hostMachineId !== machineId) so Cmd+Shift-click doesn't fire the mutation against a potentially remote workspace before locality is confirmed. - LinkHoverTooltip's getDefaultEditor catch now console.warn's the error before falling back to null, so settings RPC failures stay observable.
6 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 18, 2026
…-dnd-links upstream取り込み: v2 terminal modifier-keyed links + DnD (superset-sh#3512 + superset-sh#3542)
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 18, 2026
- 064db9f feat(desktop): modifier-keyed v2 terminal file links + folder sidebar reveal (superset-sh#3512) → PR #312 - 0b62387 fix(desktop): wire drag-and-drop for files in v2 terminal panes (superset-sh#3542) → PR #312
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Redesigns how v2 workspace terminals handle Cmd/Ctrl-click on file paths. Replaces the settings-based branch (which this PR originally shipped) with a modifier-key pattern that also adds directory sidebar reveal.
Implementation
Reuses existing machinery.
FilesTabalready has aselectedFilePath→fileTree.reveal+scrollIntoVieweffect driven by a single prop. Nothing in that path cares whether the value is a file or folder (ancestor walk is kind-agnostic, folder tree items carry the samedata-filepathattribute,isSelectedis a string equality). So I promotedselectedFilePathinpage.tsxfrom a pane-derived value to auseState, synced from the active file pane via auseEffect. Folder focus is a directsetSelectedFilePath(folderPath)— existing code handles reveal + scroll + highlight.A positive side effect: folders now flow through
getParentForCreation, so the "New File" toolbar button creates inside whichever folder the user focused via terminal Cmd-click.Three callbacks, threaded page.tsx → usePaneRegistry → TerminalPane:
onOpenFile— reuses existingopenFilePane.onRevealPath— force-opens the sidebar (viav2WorkspaceLocalState.update) andsetSelectedFilePath.onOpenExternal— wrapselectronTrpcClient.external.openFileInEditor.mutatewith an upfrontworkspaceHost.hostMachineId !== machineIdcheck and a toast, so remote workspaces get honest feedback instead of silent failure. Mirrors the guard inFilesTab.handleOpenInEditor.Sidebar force-open is intentionally scoped to exactly one caller (terminal Cmd-click on a directory). File clicks from anywhere don't force it open.
Files touched
page.tsx— promoteselectedFilePathto state + sync effect, newrevealPathandopenExternalcallbacks, workspace-host liveQuery.usePaneRegistry.tsx— accept the three callbacks, forward to<TerminalPane>.TerminalPane.tsx— new props, modifier-keyed dispatch inonFileLinkClick, drop the inline external-editor and direct pane-store calls.v1 code, DB schema, tRPC routes, and the
terminalLinkBehaviorsettings UI are untouched — v1 still honors the setting.Test plan
Summary by CodeRabbit